feat: modernize Python project by tianjianjiang · Pull Request #719 · openvanilla/McBopomofo

tianjianjiang · 2025-10-25T07:53:11Z

User description

Modernizes Python codebase to 3.12+ with type hints and clean architecture.

PR Type

Enhancement, Documentation

Description

Migrate data tools to Python 3.12+
Add typing, refactors, better I/O handling
New encoding audit CLI utility
Improve CLIs, errors, and Unicode safety

Diagram Walkthrough

flowchart LR
  tools["Data tools (compilers/builders/scripts)"] -- "py312 typing, refactors" --> io["Robust I/O & Unicode"]
  io -- "PROJECT_ROOT, encoding=utf-8" --> stability["Stability & portability"]
  tools -- "new CLI" --> audit["audit_encoding.py"]
  compilers["Compilers"] -- "functions split + typing" --> outputs["Deterministic outputs"]

File Walkthrough

Relevant files

Enhancement

12 files

score_validator.py `Py3 refactor with typing and safer I/O`	+235/-155
main_compiler.py `Decompose compiler, add loaders and typing`	+136/-185
analyze_data.py `Modular analysis with functions and CLI main`	+199/-127
text_filter.py `Regex builder modernization and py3 printing`	+47/-47
frequency_builder.py `Typed, modular frequency generation pipeline`	+74/-57
count_occurrences.py `Parallel counter with argparse and robust I/O`	+69/-65
phrase_deriver.py `Safer zipping, minor cleanup and typing`	+12/-25
map_bpmf.py `Add loaders, PROJECT_ROOT paths, safe mapping`	+53/-55
plain_bpmf_compiler.py `Extract loaders, skip pattern, f-strings`	+44/-28
audit_encoding.py `New encoder audit CLI for BPMFBase.txt`	+86/-0
compiler_utils.py `Type hints and stable float formatting`	+7/-10
playground.ipynb `Update cache usage and DataFrame vars`	+9/-269

Configuration changes

1 files

pyproject.toml `Require Python 3.12+, add ruff config and CLI`	+16/-6

Documentation

2 files

AGENTS.md `Update docs for Python 3.12 and paths`	+4/-4
README.md `Refresh data documentation and tooling notes`	+314/-102

Additional files

31 files

README	+0/-67
Makefile	+0/-6
count.bash	+0/-19
count.occurrence.c	+0/-43
DEPRECATED.md	+0/-175
README	+0/-238
build.bash	+0/-11
filter.bash	+0/-41
audit_encoding.swift	+0/-83
bpmfmap.py	+0/-63
buildFreq.py	+0/-73
cook-plain-bpmf.py	+0/-44
cook.py	+0/-251
cook_util.py	+0/-35
count.bash	+0/-11
count.occurrence.py	+0/-90
derive_associated_phrases.py	+0/-118
BIG5toUTF8.pl	+0/-20
bpmfmap_human.py	+0/-148
build4wlist.bash	+0/-5
buildFreq.bash	+0/-20
cook.rb	+0/-188
count.occurrence.pl	+0/-9
countphrase.bash	+0/-7
randomShuffle.bash	+0/-43
typocorrection.bash	+0/-2
utf8length.pl	+0/-21
nonCJK_filter.py	+0/-67
self-score-test.py	+0/-323
README.md	+0/-44
requirements.txt	+0/-4

github-actions · 2025-10-25T07:55:51Z

PR Code Suggestions ✨

Latest suggestions up to 1ef9514
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Pass missing arguments to helpers Several helper calls to `seg_pick`/walkers omit the required `phrases` argument, causing runtime errors. Ensure every helper invocation passes `phrases` consistently. This bug also appears in other walkers; fix all similar call sites. Source/Data/curation/validators/score_validator.py [125-176] def four_char_walk( phrases: dict[str, list[tuple[str, float]]], bpmf2walk: str ) -> tuple[str, float]: bpmfinput = bpmf2walk.split("-") candidate = [] # 1-2-3-4, 1-23-4, 1-2-34, 1-234 if ( bpmfinput[0] not in XXXXXXX and bpmfinput[1] not in XXXXXXX and bpmfinput[2] not in XXXXXXX and bpmfinput[3] not in XXXXXXX ): segcand = "" segscore = 0 mybpmf = bpmfinput[0] (segcand, segscore) = seg_pick(phrases, mybpmf, segcand, segscore) thisbpmf = "-".join(bpmfinput[1:4]) (a, b) = three_char_walk(phrases, thisbpmf) segcand += a segscore += b candidate.append((segcand, segscore)) # 12-3-4, 12-34 if ( bpmfinput[2] not in XXXXXXX and bpmfinput[3] not in XXXXXXX and "-".join(bpmfinput[0:2]) in phrases ): segcand = "" segscore = 0 thisbpmf = "-".join(bpmfinput[0:2]) - (segcand, segscore) = seg_pick(thisbpmf, segcand, segscore) + (segcand, segscore) = seg_pick(phrases, thisbpmf, segcand, segscore) thisbpmf = "-".join(bpmfinput[2:4]) (a, b) = two_char_walk(phrases, thisbpmf) segcand += a segscore += b candidate.append((segcand, segscore)) # 123-4 if bpmfinput[3] not in XXXXXXX and "-".join(bpmfinput[0:3]) in phrases: segcand = "" segscore = 0 thisbpmf = "-".join(bpmfinput[0:3]) - (segcand, segscore) = seg_pick(thisbpmf, segcand, segscore) + (segcand, segscore) = seg_pick(phrases, thisbpmf, segcand, segscore) mybpmf = bpmfinput[3] - (segcand, segscore) = seg_pick(mybpmf, segcand, segscore) + (segcand, segscore) = seg_pick(phrases, mybpmf, segcand, segscore) candidate.append((segcand, segscore)) # 1234 thisbpmf = "-".join(bpmfinput[0:4]) if thisbpmf in phrases: segcand = "" segscore = 0 - (segcand, segscore) = seg_pick(thisbpmf, segcand, segscore) + (segcand, segscore) = seg_pick(phrases, thisbpmf, segcand, segscore) candidate.append((segcand, segscore)) # candidate.sort(key=lambda x: x[1], reverse=True) return candidate[0] Suggestion importance[1-10]: 8 __ Why: Correct: several calls to `seg_pick` in `four_char_walk` omit the required `phrases` argument (e.g., lines where `(segcand, segscore) = seg_pick(thisbpmf, segcand, segscore)`), which would raise a TypeError. Fixing these is important for correctness in path exploration.	Medium
	Avoid strict zip to prevent crash Using `zip(..., strict=True)` will raise a ValueError on Python versions prior to 3.10 or if lengths mismatch elsewhere, causing the builder to crash. Remove `strict=True` and rely on the earlier explicit length check to keep behavior robust. Source/Data/curation/builders/phrase_deriver.py [53-55] -self._cached_zipped_readings_and_values = list(zip(reading_parts, self.value, strict=True)) +self._cached_zipped_readings_and_values = list(zip(reading_parts, self.value)) return self._cached_zipped_readings_and_values Suggestion importance[1-10]: 7 __ Why: Using `strict=True` can raise on older Python or mismatched lengths; since a length check already exists, removing `strict` improves compatibility without changing logic.	Medium
	Fix incorrect README path The `readme` path should be relative to the project root where `pyproject.toml` resides. If this file lives in `Source/Data/`, referencing `"README.md"` will likely fail packaging metadata rendering on PyPI. Point it to the in-directory README to avoid broken long description. Source/Data/pyproject.toml [9] -readme = "README.md" +readme = "Source/Data/README.md" Suggestion importance[1-10]: 2 __ Why: The `existing_code` matches line 9, but in PEP 621 the `readme` path is relative to the directory containing `pyproject.toml`, so `"README.md"` is already correct; changing to `Source/Data/README.md` would be wrong.	Low
General	Align classifiers with tested versions If the codebase isn't CI-tested and guaranteed to work on 3.13, advertising 3.13 support can mislead users. Either ensure CI covers 3.13 or remove the classifier to match actual tested versions. Source/Data/pyproject.toml [21] -"Programming Language :: Python :: 3.13", +# Remove the 3.13 classifier until verified by CI +# "Programming Language :: Python :: 3.13", Suggestion importance[1-10]: 6 __ Why: The line for Python 3.13 exists at 21; advising removal until CI verifies is reasonable quality control, though it's a process check rather than a code fix so impact is moderate.	Low
	Make README reference clickable Confirm the path is correct from repo root. If contributors open this from elsewhere (e.g., PyPI sdist or different CWD), a relative link may break. Use a repository-absolute link to ensure it's always navigable in rendered contexts. AGENTS.md [91] -For detailed dictionary data documentation, see `Source/Data/README.md` which covers file formats, editing workflows, Python tools, and troubleshooting. +For detailed dictionary data documentation, see [Source/Data/README.md](Source/Data/README.md) which covers file formats, editing workflows, Python tools, and troubleshooting. Suggestion importance[1-10]: 5 __ Why: The `existing_code` corresponds to line 91; converting an inline code path to a markdown link improves readability and navigation but is a minor documentation enhancement.	Low
	Harden file loading and parsing The signature advertises returning None but the function actually populates module-level dicts; more critically, it lacks error handling and will crash if the file is missing. Wrap file I/O in try/except and early-return on malformed lines to prevent crashes during compilation. Source/Data/curation/validators/score_validator.py [38-56] def load_bpmf_base(file_path: str) -> None: """Load base BPMF character mappings into bpmf_chars and bpmf_phrases.""" - with open(file_path, encoding="utf-8") as f: - for line in f: - if not line or line[0] == "#": - continue - elements = line.rstrip().split() - if len(elements) < 5: - continue - mykey, myvalue = elements[0], elements[1] - if mykey in bpmf_chars: - bpmf_chars[mykey].append(myvalue) - else: - bpmf_chars[mykey] = [myvalue] - if mykey in bpmf_phrases: - bpmf_phrases[mykey].append(myvalue) - else: - bpmf_phrases[mykey] = [myvalue] + try: + with open(file_path, encoding="utf-8") as f: + for line in f: + if not line or line[0] == "#": + continue + elements = line.rstrip().split() + if len(elements) < 2: + continue + mykey, myvalue = elements[0], elements[1] + bpmf_chars.setdefault(mykey, []).append(myvalue) + bpmf_phrases.setdefault(mykey, []).append(myvalue) + except OSError as e: + print(f"Error reading {file_path}: {e}") + sys.exit(1) Suggestion importance[1-10]: 4 __ Why: The proposed robustness is reasonable but targets a different file than specified and partially duplicates patterns already used elsewhere in the PR; impact is moderate and not strictly required for correctness here.	Low

Previous suggestions

Suggestions up to commit 5ec4235

Category	Suggestion	Impact
General	Handle missing BPMF mappings Emitting empty strings for unmapped characters will add extra spaces and produce misleading output. Explicitly skip or mark missing mappings to keep output consistent and detectable for downstream tools. Source/Data/scripts/map_bpmf.py [63-69] -pronunciations = [bpmf_mappings.get(char, "") for char in word] +pronunciations = [] +for char in word: + p = bpmf_mappings.get(char) + if p: + pronunciations.append(p) + else: + pronunciations.append("<UNK>") phon = " ".join([word] + pronunciations) print(phon) Suggestion importance[1-10]: 7 __ Why: Emitting placeholders instead of empty strings improves downstream detectability and prevents confusing spacing; the change is accurate and enhances robustness without altering core logic.	Medium
	Remove stray no-op access The standalone `elements[4]` expression is a no-op and likely a leftover from earlier code. Remove it to avoid confusion and potential linter errors. Keep the length check to guard index access. Source/Data/curation/compilers/main_compiler.py [68-75] def load_bpmf_base(file_path: str) -> None: ... elements = line.rstrip().split() if len(elements) < 5: continue - elements[4] mykey = elements[0] myvalue = elements[1] Suggestion importance[1-10]: 5 __ Why: The standalone `elements[4]` is indeed a no-op and should be removed; it likely came from earlier code referencing the type field. This is a minor cleanliness fix that avoids linter warnings.	Low
	Avoid unsafe auto-upgrade rules With `target-version = "py39"`, enabling UP (pyupgrade) may suggest changes incompatible with runtime constraints elsewhere. If migration is partial, temporarily exclude `UP` to avoid automated rewrites that break compatibility. Source/Data/pyproject.toml [78-80] [tool.ruff.lint] -select = ["E", "F", "I", "N", "UP", "B"] +select = ["E", "F", "I", "N", "B"] ignore = ["E501"] Suggestion importance[1-10]: 5 __ Why: The ruff lint config lines (78–80) exist and the advice to drop `UP` can prevent unintended rewrites; impact is modest and depends on project readiness for pyupgrade.	Low
Possible issue	Prevent multiple branch execution These independent if-blocks will all run when length is 6, causing multiple walks and prints. Use an if/elif chain so only the matching branch executes. This avoids redundant computation and duplicated output. Source/Data/curation/validators/score_validator.py [332-357] def chkBPMFoutput(phrases, bpmf2chk): - if len(bpmf2chk.split("-")) == 2: + parts_len = len(bpmf2chk.split("-")) + if parts_len == 2: (a, b) = twoCharWalk(bpmf2chk) (c, d) = phrases[bpmf2chk][0] if (a, b) != (c, d): print(f"{c} {d:f} {a} {b:f}") - if len(bpmf2chk.split("-")) == 3: + elif parts_len == 3: (a, b) = threeCharWalk(bpmf2chk) (c, d) = phrases[bpmf2chk][0] if (a, b) != (c, d): print(f"{c} {d:f} {a} {b:f}") - if len(bpmf2chk.split("-")) == 4: + elif parts_len == 4: (a, b) = fourCharWalk(bpmf2chk) (c, d) = phrases[bpmf2chk][0] if (a, b) != (c, d): print(f"{c} {d:f} {a} {b:f}") - if len(bpmf2chk.split("-")) == 5: + elif parts_len == 5: (a, b) = fiveCharWalk(bpmf2chk) (c, d) = phrases[bpmf2chk][0] if (a, b) != (c, d): print(f"{c} {d:f} {a} {b:f}") - if len(bpmf2chk.split("-")) == 6: + elif parts_len == 6: (a, b) = sixCharWalk(bpmf2chk) (c, d) = phrases[bpmf2chk][0] if (a, b) != (c, d): print(f"{c} {d:f} {a} {b:f}") Suggestion importance[1-10]: 6 __ Why: The existing code uses multiple independent `if` checks causing redundant work for longer inputs; switching to `elif` is a correct, low-risk micro-optimization that improves clarity. Impact is moderate since functional correctness isn’t broken today.	Low
	Fix README path for packaging Ensure `Source/Data/README.md` exists at publish time or set a path relative to this pyproject. If the file actually lives under `Source/Data/README.md`, point `readme` to the correct relative path to avoid build failures on packaging. Source/Data/pyproject.toml [9] -readme = "README.md" +readme = "Source/Data/README.md" Suggestion importance[1-10]: 6 __ Why: The snippet `readme = "README.md"` is at line 9 and is valid TOML, but the suggestion raises a reasonable packaging concern about the correct relative path; impact is moderate and context-dependent.	Low
	Correct console script module path Verify the entry point module path is importable from the package root. If `scripts/audit_encoding.py` is inside `Source/Data/`, include the package in `packages` and use its fully-qualified module path to prevent console_script import errors. Source/Data/pyproject.toml [48] -mcbpmf-audit-encoding = "scripts.audit_encoding:main" +mcbpmf-audit-encoding = "curation.scripts.audit_encoding:main" Suggestion importance[1-10]: 4 __ Why: The entry point line exists at 48, but changing it to `curation.scripts.audit_encoding` is speculative without proof of package structure; it's a valid verification note with uncertain necessity.	Low

github-actions · 2025-11-01T16:00:31Z

Persistent review updated to latest commit 1ef9514

openhands-ai · 2025-11-03T20:26:43Z

Looks like there are a few issues preventing this PR from being merged!

GitHub Actions are failing:
- Build

If you'd like me to help, just leave a comment, like

@OpenHands please fix the failing actions on PR #719 at branch `refactor/py3_migration`

Feel free to include any additional details that might help me get this PR into a better state.

_{^{You can manage your notification settings}}

…es (#733) This commit fixes critical bugs in the Python 3 modernization that were causing GitHub Actions workflow failures in the "Test McBopomofo" step. Fixes in score_validator.py: - Add missing 'phrases' parameter to 20+ seg_pick() calls (lines 155, 166, 168, 175, 198, 214, 229, 240, 242, 249, 273, 290, 306, 321, 332, 334, 341) - Add missing 'phrases' parameter to three_char_walk() calls (lines 216, 292, 308) - Fix incorrect walker function call: four_char_walk -> five_char_walk (line 275) - Optimize control flow by converting multiple if statements to elif chain in check_bpmf_output() function to prevent redundant execution Fixes in phrase_deriver.py: - Remove zip(strict=True) for Python 3.9+ compatibility (strict parameter requires Python 3.10+) - Length validation already performed before zip, so strict mode redundant All fixes verified with: - Syntax validation (py_compile) - Full data build (make all) - Data integrity checks (make check) - Score validator execution These changes resolve the TypeErrors that were preventing the workflow from completing successfully. Co-authored-by: Claude <noreply@anthropic.com>

github-actions · 2025-11-05T09:49:33Z

Claude Code Review Failed

The automated Claude review encountered an error and could not complete. You can:

Check the workflow logs for details
Trigger a manual review by commenting @claude on this PR
The review will be retried automatically on the next push

This does not affect the PR approval process.

tianjianjiang self-assigned this Oct 25, 2025

tianjianjiang changed the base branch from master to ci/workflow_path_filters_xcode_codeql October 25, 2025 07:53

github-actions Bot added the Review effort 3/5 label Oct 25, 2025

This comment was marked as outdated.

Sign in to view

Base automatically changed from ci/workflow_path_filters_xcode_codeql to master October 25, 2025 17:01

tianjianjiang force-pushed the refactor/py3_migration branch from 5ec4235 to d69e73f Compare October 26, 2025 12:47

tianjianjiang changed the title ~~refactor: python3 migration~~ feat: modernize Python project Oct 26, 2025

tianjianjiang mentioned this pull request Oct 26, 2025

ci: use Python 3.12 in Xcode workflows and exclude Source/Data from Swift/C++ CI #721

Merged

tianjianjiang force-pushed the refactor/py3_migration branch 2 times, most recently from 294bc34 to 10d184e Compare November 1, 2025 14:31

tianjianjiang mentioned this pull request Nov 1, 2025

ci: fix CodeQL build failures by using correct Python version #724

Merged

tianjianjiang force-pushed the refactor/py3_migration branch from 2aaa38e to 1cbfad2 Compare November 1, 2025 15:52

tianjianjiang changed the base branch from master to ci/fix_codeql_python_env November 1, 2025 15:52

tianjianjiang force-pushed the refactor/py3_migration branch from 1cbfad2 to 1ef9514 Compare November 1, 2025 15:57

tianjianjiang marked this pull request as ready for review November 1, 2025 15:59

tianjianjiang marked this pull request as draft November 1, 2025 15:59

github-actions Bot added Review effort 4/5 and removed Review effort 3/5 labels Nov 1, 2025

tianjianjiang force-pushed the refactor/py3_migration branch from 1ef9514 to 2203bf3 Compare November 1, 2025 16:32

tianjianjiang changed the base branch from ci/fix_codeql_python_env to master November 1, 2025 20:35

feat: modernize Python project

c1b7f61

tianjianjiang force-pushed the refactor/py3_migration branch from 2203bf3 to c1b7f61 Compare November 3, 2025 20:01

Navigation Menu

Uh oh!

feat: modernize Python project - #719

feat: modernize Python project#719
tianjianjiang wants to merge 2 commits into
masterfrom
refactor/py3_migration

tianjianjiang commented Oct 25, 2025 •

edited by github-actions Bot

Loading

Uh oh!

This comment was marked as outdated.

github-actions Bot commented Oct 25, 2025 •

edited

Loading

Uh oh!

github-actions Bot commented Nov 1, 2025

Uh oh!

openhands-ai Bot commented Nov 3, 2025

Uh oh!

github-actions Bot commented Nov 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

tianjianjiang commented Oct 25, 2025 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

This comment was marked as outdated.

github-actions Bot commented Oct 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

github-actions Bot commented Nov 1, 2025

Uh oh!

openhands-ai Bot commented Nov 3, 2025

Uh oh!

github-actions Bot commented Nov 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

tianjianjiang commented Oct 25, 2025 •

edited by github-actions Bot

Loading

github-actions Bot commented Oct 25, 2025 •

edited

Loading